Let self-managed pipx uninstall itself on windows again #1231
Conversation
guahki
force-pushed
the
fix_pipx_uninstall_windows
branch
from
268c309
to
7432157
Compare
|
I can easily add some basic tests, trying to delete a non-existing folder and an existing one. But this would not really test the "gist" of this function, the I understand, that adding new tests with each PR improves the tests and test coverage. But to request a test from me for a "bugfix" like this (obviously no test was requested, when this was broken in #1168; and most likely the test would not have found the issue, due to the facts described above) seems a little hard for me. So: I can assist with providing "basic" tests, which prove nothing (or very little) and most likely do not catch newly introduced bugs, if you want me to. But I would see it as timewaste tbh. If you want real tests, I have to ask for assistance from somebody with the time to think about, how the locked-file scenario can be tested properly. |
The point is not necessarily adding a test with each PR, it's adding a test with every bugfix to make sure we're not regressing. The point that no test was added in #1168 is not an argument against writing tests, it is a lesson that we should've done it. I appreciate this might not be easy to test. I will try and look into it a little, but as I'm not on windows it's a little cumbersome for me to play around with it. I appreciate any windows user that is willing to spend some brain power on this as well. Should we not be able to find a way to test this, IMO we can merge it, but we should do our due diligence first trying to find a meaningful test. |
Yes, I get that.
Would be very much thankful for any help here, as I unfortunately most likely will not soon have the time to develop a sensible test strategy and write sensible testcases for this one.
👍 |
|
I tried a little bit but had a hard time reproducing the behavior in a test as well. Manually, @guahki verified the behavior on windows, I did on Linux. I think this is ready for review. |
Thanks for taking the time and trying! I think one would have to have a running executable and try to delete that. Not an easy task.
Thank you for all the moderation and help. Please let me know, if there is anything to do for me on this PR. |
changelog.d/(if the patch affects the end users)Summary of changes
Change deletion of directories to ignore errors when
safe_rmis true, such that locked binaries on windows can be moved to the trash afterwards. Fixes #1203.Comment
In #1203 I proposed the following solutions:
I implemented solution 2 (see #1203 (comment) for reasoning). To be more precise, I use
ignore_errors=safe_rm, so only ignoring errors, when the left-over directory is afterwards moved to the trash.Furthermore, I removed the surrounding
try-except-block, as this (after the introduction ofignore_errors) has no effect imho:safe_rmisTrue, then errors are ignored and thus no exception should ever be raised (and potentially catched)safe_rmisFalse, in which case theexcept FileNotFoundErrorreally could make a difference. However, theif not path.is_dir():in the first line of the function should guard against this case. This is further backed by the fact, that thetry-except-block was introduced far before the safe-guardif(2aea78e 5 years ago vs. 6e1907b 3 years ago). If there is any concern about this evaluation, please let me know.Test plan
Tested by installing pipx into pipx on windows and then running
which failed before and now works